chore(angular): test schematics and code-splitting - #31401

I don't think this catches the case it's meant to. The set unions both components together, and main.js counts as a chunk, so the count can be 2 while a component sits in the initial bundle.

I tried it by importing IonToggle eagerly into AppComponent. The toggle ends up in main.js, so every landing page visitor downloads it, and this still passes. Checking that the chunk holding ion-toggle isn't one the landing page pulls in would be harder to fool.

The baseline check has the opposite problem, since requiring exactly 1 asserts the broken state is still broken. If esbuild ever splits the barrel on its own, this goes red on an improvement.

What would be a good way to check which chunks the landing page pulls in?

What should it do if esbuild splits the baseline on its own? If that silently starts passing, this test would be asserting nothing.

Built the fixture and confirmed the gap. A healthy migrated build puts the two components in two separate chunks and passes. Importing IonToggle eagerly into AppComponent, so every landing page visitor downloads it, puts the toggle in main.js and leaves the header in its own chunk. Still two chunks, so this stays green.

For what the landing page pulls in, the metafile already has enough. Find the output whose inputs include landing.page.ts, walk its imports transitively, add main.js since that's always loaded, then check the toggle isn't anywhere in that closure while the header is. That fails on the eager case above.

On the baseline, I'd stop asserting on it. Log it for context and let the migrated check carry the test, otherwise the day esbuild starts splitting the barrel on its own this goes red for an improvement.

Using PreloadAllModules here undoes what the fixture is meant to show. The router fetches every lazy route after the first navigation, so sitting on the landing page pulls down the home page chunk, toggle included.

Drop withPreloading and rebuild the same source, and the landing page loads its own chunk and leaves the home one alone. So the bundler splits it correctly and the router downloads it all anyway. Could this come out?

I'm not sure if I'm seeing an improvement before vs. after removing withPreloading. What are you looking at to verify it?

You're not seeing it in the build output because there isn't a difference there. The chunks come out the same either way, so what changes is only what the browser fetches.

Loading the landing page without preloading pulls six scripts, and the toggle chunk isn't one of them. With PreloadAllModules put back it pulls seven, the extra one being the chunk that holds the toggle, arriving just after the page settles. So it's the network panel rather than the build output.

Removing it was the right call.

The glob doesn't clear old tarballs first the way sync.sh and sync-and-pack.sh both do, and never deletes the one it just made. With 8.8.2, a nightly 9.0.0-dev and 9.0.0 all sitting there the shell hands over 8.8.2 first, and since the CLI takes one collection argument with strict(false), the extras go by silently. A leftover tarball from main means this passes having checked the wrong version.

Exiting 0 is also the only assertion here, and most rules in schematics/add/index.ts no-op rather than throw. With no app.config.ts, addProvideIonicAngular skips quietly, and addIonicStyles writes eleven hardcoded css paths nothing verifies. FW-7692 wants this to fail when schematics files are incorrect. Only missing ones get caught, and running npm run build in the generated project would pick up the css list for free.

Added steps to remove old tarballs and run a build in the generated project.

The build step is a good addition, it picks up the hardcoded css list for free.

That cleanup line doesn't do anything though. It takes a literal path rather than a glob, so with two tarballs sitting there both survive, and there's no cwd either so it resolves against process.cwd() instead of the package root. Same CWD-relative trap as clean.js on the exports PR. So ng add can still end up with more than one tarball on the command line.

The fixture's own sync.sh has the version that works, rm -f *.tgz. Worth deleting the tarball this run creates as well, next to where the test project gets cleaned up.

Reproduced this one: label belongs to build(), not to this function, so the missing stats path throws a ReferenceError instead of the message. Taking it as an argument and passing 'baseline' and 'migrated' at the two call sites would sort it.

I just removed it from the string; the build type is already announced in another console.log.

The six subpaths above this are bare kebab-case and needed no exclusion. This one takes the ion- prefix but it's a DI token rather than a component, and that's the only reason KNOWN_NON_CORE_ION_COMPONENTS had to be added to the verifier I asked for on the exports PR.

It also points at angular-delegate.js, which exports more than the token. Running the codemod with --print-map sends AngularDelegate and attachView to @ionic/angular/ion-modal-token, so someone's AngularDelegate import gets rewritten to a path named after a modal token.

Calling it ./modal-token matches the siblings and lets the exclusion list go away. Free to change now, breaking once it's released.

I'm confused about this one, AngularDelegate and attachView don't seem to be exported for the standalone components API. Should I make a barrel file that only exports IonModalToken?

The other import paths match the imported class name. It seems confusing to have to import IonModalToken from @ionic/angular/modal-token when everything else would have you expect @ionic/angular/ion-modal-token.

You're right on both counts, and my suggestion was wrong. The ionic-route-strategy entry right below it is kebab-case of the exported name too, so ion-modal-token follows the same rule. And nobody can import AngularDelegate from the barrel anyway, since the main entry resolves to the standalone index which only re-exports the token, so the codemod can't rewrite it in real code even though it turns up in the printed map.

The bit that does still stand is smaller. This subpath publishes angular-delegate.js as an entry point, so AngularFrameworkDelegate, attachView and bindLifecycleEvents become importable from it too, and that's public API once it's released. It's also why the verifier needed the new non-core whitelist. A barrel file exporting just the token, like you suggested, would sort both.

Every other package in build.yml keeps the build action to building and archiving, with the suites in their own job. This adds an ng new, two npm installs and two production Angular builds to it.

Since test-angular-e2e needs build-angular, a flaky ng new now skips all five e2e apps and shows up as build-angular failing, and stencil-nightly.yml uses the same action, so an Angular CLI or registry problem would surface as a Stencil nightly failure. It also sits after Check Diff, and both scripts write into the tree.

Would a test-angular-package job with needs: [build-angular] work instead? Tiny nit: trailing space in the step name.

Added an action for test-angular-package that build.yml runs.

Nothing runs these. No workflow touches the app's test target, and test-code-split.js only does npm i, sync.sh and npm run build, so the spec files, test-setup.ts and the vitest and jsdom devDeps are all unreachable. They only assert toBeTruthy() as well, so even wired up they wouldn't cover what this PR protects.

Looks like ng new scaffolding that came along for the ride, so worth either wiring up or dropping.

While you're in there, test-setup.ts explains its matchMedia polyfill with ion-menu and ion-split-pane, and this app uses neither.

Removed all these generated test files.

The files went but the scaffolding around them didn't. The test target in angular.json still lists the deleted setup file in setupFiles, and tsconfig.spec.json still includes it, so npm test in this app now points at a file that isn't there. The ng test script, tsconfig.spec.json and the vitest and jsdom devDependencies can all go with it.

While you're in there, the Capacitor config, ionic.config.json, the environments folder and the production fileReplacements entry are all start-template scaffolding this fixture doesn't use either.

I think this is the FW-7695 part of the PR, but I can't find the rest of it. That ticket asks for the standalone e2e pages to move to per-component imports and for this script to check every export subpath resolves from the barrel too, and the standalone pages are all still on the barrel.

The code-split app does typecheck the entry points for the seven components it uses, which is great, but it's not quite the same coverage. Is FW-7695 meant to be a follow-up?

Nit: double space before the =, and the array's missing a semicolon and trailing comma compared to the one above it. Nothing will catch those since scripts is ignored by eslint and prettier.

I wanted to use the existing test app instead of making a new code-split test, but especially with the lazy-loaded side of the app included it's way too big and complicated.

Since your last review I did add a couple steps to test-angular-e2e that runs the import migration script and re-run the tests.

Maybe the migration script should error if anything that's not a type import didn't get replaced?

Yeah, though I'd scope it narrower than anything unreplaced, since unmapped is legitimately non-empty for symbols with no dedicated entry point. The case that matters is rewriting nothing at all.

Ran into this by accident: pointed the codemod at a build from before this PR's seven new subpaths, and because ionic-route-strategy wasn't among them it left one bare barrel import in main.ts, exited 0, and every Ionic component ended up in main.js. The split was gone and the script reported success. A flag that exits non-zero when zero statements were rewritten, passed in the workflow step, would catch that along with map drift and running from the wrong directory.

Those seven new subpaths turn out to be load-bearing for the whole test, which I hadn't realised.

Separately, nothing in the repo says how to run any of this. The test README covers the build.sh flow and this fixture deliberately sits outside it, so someone hitting a red test-angular-package has nowhere to look.

A // comment inside a multi-line import takes the rest of the statement with it, since the whitespace gets collapsed before the split on commas. Feeding it this:

import {
  IonHeader, // page chrome
  IonToggle,
} from '@ionic/angular';

produces import { // page chrome IonToggle } from '@ionic/angular'; plus a correct separate line for IonHeader. The comment swallows the closing brace and the from clause, so IonToggle disappears and the build dies with TS1003 and friends.

This seems like a big issue since nothing checks the output before writing, so the first sign is a compile error somewhere unrelated. Stripping comments out of the brace body before splitting would cover it, and I think it's worth bailing on the statement rather than rewriting when something unexpected survives.

Despite the header, this never reads the target project. Node's self-reference resolution beats paths here, because the script sits inside a package named @ionic/angular whose exports map includes ./package.json. Passing a directory that doesn't exist still hands back this repo's own manifest, so the walk below never runs. CI doesn't catch it because both call sites point at an app synced from the same build, so the two agree by accident.

Dropping the require.resolve branch and keeping the walk would make the comment true.

Also, scripts/ isn't in files, so this never ships, same limitation as the schematics path on the exports PR. Is it meant to be run by consumers? The header reads like end-user docs.

This job pulls down CoreBuild.zip but the schematics test never gets to it. The ng add step installs the packed @ionic/angular, and its @ionic/core dependency is ^9.0.0, so the generated project resolves core from the registry rather than from the PR, 9.0.1 today.

That means the new build step checks this PR's Angular against released core. A PR that adds a core export the Angular types reference fails here with a confusing "has no exported member", and a core change that breaks Angular goes green.

A plain npm run sync won't fix it, since the generated project is its own npm install. I think verify-schematics.js would need to pack core and install that tarball into the test project before the build, roughly what the fixture's sync.sh already does.

Thanks for splitting this out, the job shape looks right now.

Nit: the name and description are still Build Ionic Angular from the action this was copied from, where the siblings are all Test X. The trailing space I mentioned on the old step came across with it. Up to you!

Nit: both test steps have the same name, so a failure doesn't say which import style broke. Maybe tag them "(barrel imports)" and "(per-component imports)"? No worries if you'd rather leave it.

Nit: looks like withPreloading and PreloadAllModules are left over from before the call came out. Nothing will flag them, since TypeScript elides unused imports and this tree is skipped by eslint.